-
-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement basic multivariate polynomial species #38446
Conversation
…groups Also added various miscellaneous functions
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
Also some fixes to _element_constructor_ ConjugacyClassesOfSubgroups
Added _repr_ for ConjugacyClassesOfSubgroups I now output B[(gens or name if available)] as _repr_ for generators, for example B[1] + 2*B[(2,3,4)]
…y and doctest updates
…ial case in construct_element
src/sage/rings/species.py
Outdated
def __classcall__(cls, *args, **kwds): | ||
""" | ||
Normalize the arguments. | ||
|
||
EXAMPLES:: | ||
|
||
sage: from sage.rings.species import MolecularSpecies | ||
sage: MolecularSpecies("X,Y") is MolecularSpecies(["X", "Y"]) | ||
True | ||
|
||
sage: MolecularSpecies("X,Y") == MolecularSpecies(["X", "Z"]) | ||
False | ||
""" | ||
if isinstance(args[0], AtomicSpecies): | ||
indices = args[0] | ||
else: | ||
assert "names" not in kwds or kwds["names"] is None | ||
indices = AtomicSpecies(args[0]) | ||
category = Monoids().Commutative() & SetsWithGrading().Infinite() | ||
return super().__classcall__(cls, indices, | ||
prefix='', bracket=False, | ||
category=category) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks actually fishy to me. I really would like to have MolecularSpecies
only accept names as argument. However, if I understand correctly, MolecularSpecies.__init__
has to have the same signature, but apparently it must also have the same signature as IndexedFreeAbelianMonoid.__classcall__
(or __init__
?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you can explicitly skip that __classcall__
and go to its base class's one through UniqueRepresentation.__classcall__(cls, names)
. The __init__
and __classcall__
signatures (which can be different) only need to be able to take as input the final key/signature that is given to the UniqueRepresentation
.
I think that I adressed all other comments, please let me know if this is not the case. Most importantly: many thanks for the thorough review, I appreciate it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially this is good to go modulo one doc detail and one potential change for you.
src/sage/rings/species.py
Outdated
def __classcall__(cls, *args, **kwds): | ||
""" | ||
Normalize the arguments. | ||
|
||
EXAMPLES:: | ||
|
||
sage: from sage.rings.species import MolecularSpecies | ||
sage: MolecularSpecies("X,Y") is MolecularSpecies(["X", "Y"]) | ||
True | ||
|
||
sage: MolecularSpecies("X,Y") == MolecularSpecies(["X", "Z"]) | ||
False | ||
""" | ||
if isinstance(args[0], AtomicSpecies): | ||
indices = args[0] | ||
else: | ||
assert "names" not in kwds or kwds["names"] is None | ||
indices = AtomicSpecies(args[0]) | ||
category = Monoids().Commutative() & SetsWithGrading().Infinite() | ||
return super().__classcall__(cls, indices, | ||
prefix='', bracket=False, | ||
category=category) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you can explicitly skip that __classcall__
and go to its base class's one through UniqueRepresentation.__classcall__(cls, names)
. The __init__
and __classcall__
signatures (which can be different) only need to be able to take as input the final key/signature that is given to the UniqueRepresentation
.
Cool, that worked and looks much better! Thank you! |
src/sage/rings/species.py
Outdated
INPUT: | ||
|
||
- ``dis`` -- a directly indecomposable permutation group | ||
- ``domain_partition`` -- a `k`-tuple of ``frozenset``s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ``domain_partition`` -- a `k`-tuple of ``frozenset``s, | |
- ``domain_partition`` -- a `k`-tuple of ``frozenset`` entries, |
This will not format properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, should all of this information should be moved to the (public facing) class level?
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
src/sage/rings/species.py
Outdated
H = _stabilizer_subgroups(S, X, a) | ||
if len(H) > 1: | ||
raise ValueError("action is not transitive") | ||
return self(H[0], pi, check=check) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return self(H[0], pi, check=check) | |
return self._element_constructor_(H[0], pi, check=check) |
This is where you expect it to always end up (without any coercion being done), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I reorganized so that recursion is avoided, because it is a pain when debugging.
Also, the error message was (mathematically) wrong.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Thank you for being careful, I especially like that we caught the wrong error message! |
Thank you for all the changes. If the bot comes back (morally) green, then you can set a positive review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved via tscrim
yippie! |
Related to sagemath#30727. We implement basic functionality for multivariate polynomial species, using its representation as a pair of a permutation group and a mapping between the domain of the permutation group and some variables. We provide addition, multiplication, and (partitional) composition (for some special cases). We also allow it to be constructed as a group action (or a sequence thereof). Atomic and molecular decompositions are automatically computed thanks to sagemath#38371. ### 📝 Checklist - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. URL: sagemath#38446 Reported by: Mainak Roy Reviewer(s): Mainak Roy, Martin Rubey, Travis Scrimshaw
Related to sagemath#30727. We implement basic functionality for multivariate polynomial species, using its representation as a pair of a permutation group and a mapping between the domain of the permutation group and some variables. We provide addition, multiplication, and (partitional) composition (for some special cases). We also allow it to be constructed as a group action (or a sequence thereof). Atomic and molecular decompositions are automatically computed thanks to sagemath#38371. ### 📝 Checklist - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. URL: sagemath#38446 Reported by: Mainak Roy Reviewer(s): Mainak Roy, Martin Rubey, Travis Scrimshaw
See https://mathoverflow.net/questions/480823/the-tilde-species Dependencies: sagemath#38446 URL: sagemath#38883 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton
See https://mathoverflow.net/questions/480823/the-tilde-species Dependencies: sagemath#38446 URL: sagemath#38883 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton
See https://mathoverflow.net/questions/480823/the-tilde-species Dependencies: sagemath#38446 URL: sagemath#38883 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton
Related to #30727.
We implement basic functionality for multivariate polynomial species, using its representation as a pair of a permutation group and a mapping between the domain of the permutation group and some variables. We provide addition, multiplication, and (partitional) composition (for some special cases). We also allow it to be constructed as a group action (or a sequence thereof). Atomic and molecular decompositions are automatically computed thanks to #38371.
📝 Checklist